Skip to content

Ignore pods with status.phase=Succeeded when watching IP addresses#5412

Merged
kleimkuhler merged 1 commit intolinkerd:mainfrom
fpetkovski:ld-5394
Jan 12, 2021
Merged

Ignore pods with status.phase=Succeeded when watching IP addresses#5412
kleimkuhler merged 1 commit intolinkerd:mainfrom
fpetkovski:ld-5394

Conversation

@fpetkovski
Copy link
Contributor

@fpetkovski fpetkovski commented Dec 19, 2020

Ignore pods with status.phase=Succeeded when watching IP addresses

When a pod terminates successfully, some CNIs will assign its IP address
to newly created pods. This can lead to duplicate pod IPs in the same
Kubernetes cluster.

Filter out pods which are in a Succeeded phase since they are not
routable anymore.

Fixes #5394

@fpetkovski fpetkovski requested a review from a team as a code owner December 19, 2020 11:11
@fpetkovski
Ignore pods with status.phase=Succeeded when watching IP addresses
7ea95f8 
When a pod terminates successfully, some CNIs will assign its IP address
to newly created pods. This can lead to duplicate pod IPs in the same
Kubernetes cluster.

Filter out pods which are in a Succeeded phase since they are not
routable anymore.

Fixes linkerd#5394

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>
fpetkovski
fpetkovski commented Dec 21, 2020
View reviewed changes
controller/api/destination/watcher/ip_watcher.go

// Ignore terminated pods,
// their IPs can be reused for new Running pods
if podTerminated(pod) {
Copy link
Contributor Author

@fpetkovski fpetkovski Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure if this change is need. Looking forward to any feedback

Copy link
Member

@adleong adleong Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to omit terminated pods any time we are using the pod ip index. So yes, I think this makes sense.

@adleong
Copy link
Member

adleong commented Dec 21, 2020

This looks great to me. @kforsthoevel are you able to test this branch and confirm if it fixes #5394 for you?

@kforsthoevel
Copy link

kforsthoevel commented Dec 24, 2020

Thanks. We will try it out after the holidays.

@thomaspeitz
Copy link

thomaspeitz commented Jan 5, 2021

@adleong - Is there any chance that you improve your build process by pushing the branch images? - Or are they already pushed and I do not see them. - The ones used during building seem to be tagged correctly but not pushed.

That would help us a lot to test it. Else we would build / push those images ourselves.

Greetings :)

@kleimkuhler
Copy link
Contributor

kleimkuhler commented Jan 5, 2021

@thomaspeitz It's not exactly what you're asking for, but we do have something like this available if you clone the repo.

There is a script bin/install-pr that will install the images uploaded during the CI process and load them into a local cluster. We use this for testing PRs which sounds like what you're trying to do here.

For this PR, I can test in a k3d cluster with the following:

❯ env GITHUB_TOKEN='..' bin/install-pr --k3d 5412
..
INFO[0004] DONE                                         
/home/kevin/Projects/linkerd/linkerd2

Linkerd CLI available:
/home/kevin/Projects/linkerd/linkerd2/target/release/linkerd2-cli-git-65f0d802-linux-amd64

Then you can install it and test:

./target/release/linkerd2-cli-git-65f0d802-linux-amd64 install |kubectl apply -f -

The value of GITHUB_TOKEN is your GitHub personal access token. If you don't pass --k3d or --kind, it assumes a regular k8s cluster.

@fpetkovski
Copy link
Contributor Author

fpetkovski commented Jan 5, 2021 •
edited
Loading

The way I tested the change was by first running the bin/docker-build script which creates a unique tag based on the currently checked out commit sha. I then retagged the destination image and pushed it to a public dockerhub repo, from where I could get it pulled into a remote, non-production Kubernetes cluster.

@thomaspeitz
Copy link

thomaspeitz commented Jan 5, 2021

Thanks for your input!

We will continue tomorrow and give you feedback as soon as possible.

@thomaspeitz
Copy link

thomaspeitz commented Jan 6, 2021

We upgraded to newest edge version, replaced the destination container with the updated controller image and now monitor closely logs in case any problem happen. We get back to you beginning next week with test results (or earlier in cases of problems). Thanks for your work! :)

@kleimkuhler kleimkuhler mentioned this pull request Jan 11, 2021
Closed
@kforsthoevel
Copy link

kforsthoevel commented Jan 12, 2021

@adleong, @fpetkovski The patch is running for almost a week now and we haven't see the error anymore. It seems to have fixed the issue.

Thank you for the quick fix.

kleimkuhler
kleimkuhler approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @fpetkovski

adleong
adleong approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks @kforsthoevel for confirming!

@kleimkuhler kleimkuhler merged commit 40192e2 into linkerd:main Jan 12, 2021
@kivagant-ba
Copy link

kivagant-ba commented Jan 12, 2021

What version includes the fix?

@kleimkuhler
Copy link
Contributor

kleimkuhler commented Jan 12, 2021

This only recently merged so it is not part of a release yet. We do edge releases at the end of each week, so edge-20.1.2 will include it, and that should be available by Thursday or Friday.

@kivagant-ba
Copy link

kivagant-ba commented Jan 12, 2021 •
edited
Loading

Another question: can the fix be included as a hotfix into last couple of stable versions without having everyone to upgrade to a release with all of the new functionality? It is a quite critical bug that causes issues when pods cannot even start because liveness probes cannot pass. And this probably causes hidden issues when traffic cannot be balanced properly (I guess this is what is happening when we see the WARNs in logs).

@kleimkuhler
Copy link
Contributor

kleimkuhler commented Jan 13, 2021

@kivagant-ba Sure, we'll release a stable-2.9.2 with this fix. I'll leave a comment here when it has been released; I'm going to start prepping it today.

kleimkuhler pushed a commit that referenced this pull request Jan 13, 2021
@fpetkovski @kleimkuhler
Ignore pods with status.phase=Succeeded when watching IP addresses (#…
f201d78 
…5412)

Ignore pods with status.phase=Succeeded when watching IP addresses

When a pod terminates successfully, some CNIs will assign its IP address
to newly created pods. This can lead to duplicate pod IPs in the same
Kubernetes cluster.

Filter out pods which are in a Succeeded phase since they are not 
routable anymore.

Fixes #5394

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>
kleimkuhler pushed a commit that referenced this pull request Jan 13, 2021
@fpetkovski @kleimkuhler
Ignore pods with status.phase=Succeeded when watching IP addresses (#…
726abf2 
…5412)

Ignore pods with status.phase=Succeeded when watching IP addresses

When a pod terminates successfully, some CNIs will assign its IP address
to newly created pods. This can lead to duplicate pod IPs in the same
Kubernetes cluster.

Filter out pods which are in a Succeeded phase since they are not 
routable anymore.

Fixes #5394

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>
kleimkuhler pushed a commit that referenced this pull request Jan 13, 2021
@fpetkovski @kleimkuhler
Ignore pods with status.phase=Succeeded when watching IP addresses (#…
015c470 
…5412)

Ignore pods with status.phase=Succeeded when watching IP addresses

When a pod terminates successfully, some CNIs will assign its IP address
to newly created pods. This can lead to duplicate pod IPs in the same
Kubernetes cluster.

Filter out pods which are in a Succeeded phase since they are not
routable anymore.

Fixes #5394

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
@kleimkuhler
Copy link
Contributor

kleimkuhler commented Jan 13, 2021

stable-2.9.2 has been released with this fix.

@kivagant-ba
Copy link

kivagant-ba commented Jan 14, 2021

Thank you, @kleimkuhler !

@kleimkuhler kleimkuhler mentioned this pull request Mar 1, 2021
Closed
@Wenliang-CHEN Wenliang-CHEN mentioned this pull request Mar 23, 2021
Closed
This was referenced Feb 27, 2026
Closed
Merged
@clubanderson clubanderson mentioned this pull request Mar 11, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adleong adleong adleong approved these changes

+1 more reviewer

@kleimkuhler kleimkuhler kleimkuhler approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Traffic stops when two pods have the same IP address

6 participants

@fpetkovski @adleong @kforsthoevel @thomaspeitz @kleimkuhler @kivagant-ba